feat: add one-click copy to clipboard for remote connection URL#360
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Copy Link button to the Settings page connection URL, with clipboard write support, a fallback copy path, and localized success or error feedback. ChangesCopy Link Button for Share URL
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage
participant Clipboard as navigator.clipboard
User->>SettingsPage: click Copy Link
SettingsPage->>Clipboard: writeText(shareUrl)
alt success
Clipboard-->>SettingsPage: resolved
SettingsPage->>SettingsPage: set copied = true
SettingsPage->>SettingsPage: reset after 2 seconds
else failure
Clipboard-->>SettingsPage: rejected
SettingsPage->>SettingsPage: set copyError
end
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/settings.tsx`:
- Line 392: The button labels "Copied! ✅" and "Copy Link" on line 392 in
src/routes/settings.tsx are hardcoded user-facing strings that should be
externalized to i18n resources for consistency and translatability. Move these
string literals into the appropriate i18n/localization resource file with
meaningful keys (e.g., copyLinkSuccess and copyLinkButton), then replace the
hardcoded strings in the JSX with references to those i18n keys using the
standard i18n translation function from your project (typically useTranslation
hook or similar), ensuring the ternary expression now references the translated
strings instead of inline literals.
- Around line 386-390: The onClick handler calls
navigator.clipboard.writeText(shareUrl) which returns a Promise, but this
Promise is not awaited and the success state setCopied(true) is set immediately
regardless of the clipboard operation's outcome. To fix this, convert the
onClick handler to an async function and await the clipboard.writeText() call
before setting the copied state, ensuring setCopied(true) only executes after
the clipboard write succeeds. Additionally, add error handling to gracefully
handle cases where the clipboard operation fails due to permission denial or
other issues.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c258009-5ca2-4339-8cc3-08fc4f4a33e2
📒 Files selected for processing (1)
src/routes/settings.tsx
|
@coderabbitai I have updated the code to await the Regarding the i18n suggestion: The project currently does not use any i18n localization framework (all other strings in |
|
Tip For best results, initiate chat on the files or code changes.
The implementation looks solid. Thanks for the thorough explanation! 🐇 ₍^·ᴥ·^₎ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
Dotify71
force-pushed
the
feat/copy-to-clipboard
branch
from
c110429 to
130beb1
Compare
|
Thanks for the PR. I’ve gone through it. Could you externalize the string into the file as CodeRabbit suggested? Also, I think it would be better to remove the emoji from the copied string. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/settings.tsx (1)
368-382: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard clipboard copy on non-secure LAN sessions
navigator.clipboardis unavailable in insecure HTTP contexts, so this button will fail on the LAN/remote setup the page is built for. Add a secure-context check with a fallback copy path, and show a visible error if copying still fails.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/settings.tsx` around lines 368 - 382, The copyLink button in settings.tsx currently assumes navigator.clipboard is always available, which breaks in insecure HTTP/LAN sessions. Update the onClick handler in the settings share URL button to first check for a secure context before calling navigator.clipboard.writeText, and add a fallback copy path for unsupported cases. If both the primary and fallback copy attempts fail, surface a visible user-facing error instead of only logging in the catch block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/routes/settings.tsx`:
- Around line 368-382: The copyLink button in settings.tsx currently assumes
navigator.clipboard is always available, which breaks in insecure HTTP/LAN
sessions. Update the onClick handler in the settings share URL button to first
check for a secure context before calling navigator.clipboard.writeText, and add
a fallback copy path for unsupported cases. If both the primary and fallback
copy attempts fail, surface a visible user-facing error instead of only logging
in the catch block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: be24b280-e8ec-4b73-a0be-ad7b2e50fed2
📒 Files selected for processing (2)
src/routes/settings.tsxsrc/utils/i18n.ts
|
@coderabbitai can you give a review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd settings.tsx
echo "---"
fd i18n.ts
echo "---"
cat -n src/routes/settings.tsx | sed -n '1,200p'
echo "---i18n---"
cat -n src/utils/i18n.ts | sed -n '1,80p'Length of output: 9298 🏁 Script executed: #!/bin/bash
sed -n '260,340p' src/routes/settings.tsxLength of output: 2644 🏁 Script executed: #!/bin/bash
grep -n "shareUrl\|copyLink\|copied\|Copy Link\|clipboard\|copyWithFallback\|copyError" src/routes/settings.tsxLength of output: 1024 🏁 Script executed: #!/bin/bash
sed -n '370,420p' src/routes/settings.tsxLength of output: 1804
Additionally, I noticed the implementation now also includes a nice touch beyond what was requested:
This looks solid overall. Nice work incorporating the feedback! 🐇 ₍^•ᴥ•^₎ ✏️ Learnings added
✅ Action performedReview finished.
|
|
@PinJinx i have done some changes can you review this pr now |
|
@Dotify71 Just fix the formatting error and ping me, I’ll merge the PR. Thanks for your contribution! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/settings.tsx`:
- Around line 156-171: Guard the copy fallback cleanup in copyWithFallback so
the temporary textarea is always removed even if document.execCommand("copy")
throws. Wrap the execCommand call and the removal logic in a try/finally block,
and keep the cleanup tied to the copyWithFallback helper so the DOM node is
deleted on both success and failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bb172c2-a521-4f56-8268-4dfc9769271f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/routes/settings.tsxsrc/utils/i18n.ts
|
@PinJinx i think everything is set now |
|
@PinJinx can you re-run the CI/CD |
|
@Dotify71 Why did you edit the workflow file? Could you explain the reasoning behind that change? I’d suggest keeping the PR limited to only the required changes. |
|
Hey @PinJinx! I was initially seeing some flaky behavior with the Windows CI build locally and in the checks (npm ci was failing on Windows due to optional dependencies/package-lock issues), so I tried tweaking the workflow to get the green checkmark. However, I completely agree with you, it's best to keep this PR strictly scoped to the 'copy to clipboard' feature and not mix in CI changes. I have just reverted the workflow file changes back to main. Thanks for the review and let me know if there's anything else! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/settings.tsx (2)
156-173: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove
copyWithFallbackoutside the component.This function takes only
textas a parameter and uses no component state or props, so defining it insideSettingsPagerecreates it on every render. It also blocks React Compiler auto-memoization of the entire component — the static analysis hint flags thetry/finallywithout acatchclause at line 167 as an unsupported statement for the compiler's HIR lowering. Moving it to module scope fixes both issues.♻️ Proposed refactor
+const copyWithFallback = (text: string) => { + const textArea = document.createElement("textarea") + textArea.value = text + textArea.setAttribute("readonly", "") + textArea.style.position = "absolute" + textArea.style.left = "-9999px" + + document.body.appendChild(textArea) + textArea.select() + textArea.setSelectionRange(0, text.length) + + try { + return document.execCommand("copy") + } finally { + document.body.removeChild(textArea) + } +} + function SettingsPage() { const [ip, setIp] = useState("") const [copied, setCopied] = useState(false) const [copyError, setCopyError] = useState("") - const [frontendPort, setFrontendPort] = useState("") + const [frontendPort, setFrontendPort] = useState("")Then remove the original definition at lines 156–173.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/settings.tsx` around lines 156 - 173, Move copyWithFallback out of SettingsPage into module scope so it is not recreated on every render and no longer blocks React Compiler auto-memoization. Keep the function behavior the same, but define it alongside the other top-level helpers in settings.tsx and remove the inline definition inside the component; use the existing copyWithFallback symbol wherever the component currently calls it.Source: Linters/SAST tools
404-405: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the previous copy timeout before scheduling a new one.
If the user clicks "Copy Link" twice within 2 seconds, the first
setTimeoutstill fires and prematurely resetscopiedtofalse, causing the "Copied!" label to flicker off even though the second copy just succeeded. The timer is also not cleared on unmount.🐛 Proposed fix
Add a ref to track the timeout and clear it before scheduling:
+ const copyTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null) + // ... inside the onClick handler, after setCopied(true): - setCopied(true) - setTimeout(() => setCopied(false), 2000) + setCopied(true) + if (copyTimerRef.current) clearTimeout(copyTimerRef.current) + copyTimerRef.current = setTimeout(() => { + setCopied(false) + copyTimerRef.current = null + }, 2000)And clear it in the
catchblock:} catch (err) { console.error("Failed to copy URL:", err) + if (copyTimerRef.current) { + clearTimeout(copyTimerRef.current) + copyTimerRef.current = null + } setCopied(false) setCopyError(t("settings", "copyFailed")) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/settings.tsx` around lines 404 - 405, The copy feedback timer in the settings copy handler is not being managed, so repeated clicks can let an older timeout reset copied too early and it may also leak on unmount. Update the Copy Link logic in settings.tsx to store the timeout in a ref, clear any existing timeout before scheduling a new one after setCopied(true), and clear the pending timeout in the cleanup/unmount path and any error handling path related to the copy action.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/routes/settings.tsx`:
- Around line 156-173: Move copyWithFallback out of SettingsPage into module
scope so it is not recreated on every render and no longer blocks React Compiler
auto-memoization. Keep the function behavior the same, but define it alongside
the other top-level helpers in settings.tsx and remove the inline definition
inside the component; use the existing copyWithFallback symbol wherever the
component currently calls it.
- Around line 404-405: The copy feedback timer in the settings copy handler is
not being managed, so repeated clicks can let an older timeout reset copied too
early and it may also leak on unmount. Update the Copy Link logic in
settings.tsx to store the timeout in a ref, clear any existing timeout before
scheduling a new one after setCopied(true), and clear the pending timeout in the
cleanup/unmount path and any error handling path related to the copy action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97880ea1-322c-4364-ae3e-e7ab9267e891
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/routes/settings.tsx
…r and function scope
|
@coderabbitai can you give a review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -10
echo "---"
git diff origin/main -- .github/workflows | head -50
echo "---"
cat -n src/routes/settings.tsx | sed -n '1,20p'
echo "---"
sed -n '380,430p' src/routes/settings.tsxLength of output: 3567 🏁 Script executed: #!/bin/bash
sed -n '20,40p' src/routes/settings.tsx
echo "---"
sed -n '430,450p' src/routes/settings.tsx
echo "---i18n---"
grep -n "settings" -A5 src/utils/i18n.ts | head -20
echo "---package-lock diff---"
git diff origin/main -- package-lock.json | head -30Length of output: 2558
One small FYI (not blocking): Otherwise, this PR looks ready to merge. Nice work addressing all the feedback! 🐇 ₍^•ᴥ•^₎ ✅ Action performedReview finished.
|
|
@PinJinx I've fixed the Windows CI failure by cleanly regenerating the I also went ahead and addressed the recent review feedback (moved the copy fallback function out of the component scope for better memoization and fixed the copy button's timeout flicker issue). Could you please trigger the CI/CD run again when you have a moment? Everything should be green now! Thanks! |
…m optional bindings The previous lockfile regeneration dropped optional dependencies for Windows and Linux because it was generated from scratch on macOS. This incrementally updates the lockfile from main to include the new dependencies without losing the OS-specific bindings.
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
Updated shell-quote to 1.8.4 via npm audit fix to resolve GHSA-w7jw-789q-3m8p.
|
I've resolved the critical CVE by updating For the other two warnings regarding @SocketSecurity ignore npm/env-runner@0.1.10 |
|
@PinJinx can you re-run |
|
@Dotify71 Just remove the package lock only keep the changed files and I am sure it will pass all the test |
|
Done as you said @PinJinx |
|
There is a formatting error that's why the ci validate is failing, fix the formatting of just that file and run npx @biomejs/biome check to make sure it works |
|
Done @PinJinx |
2 participants
Fixes #359
Description
Currently, users who cannot scan the QR code have to manually highlight and copy the long IP address + auth token URL on their desktop to share it to their device. This is tedious on some screens or setups where dragging to highlight is difficult.
Solution
Added a "Copy Link" button beneath the URL on the settings page with a brief "Copied! ✅" visual confirmation, greatly improving setup UX.
navigator.clipboard.writeTextSummary by CodeRabbit